fix(rust): Centralize FixedSizeList outer-to-child layout arithmetic and fix under-reservation - #28993
fix(rust): Centralize FixedSizeList outer-to-child layout arithmetic and fix under-reservation#28993Angshuman09 wants to merge 3 commits into
Conversation
|
#28980 already addresses #28872 and was under maintainer review before this PR was opened. I created #28872 and #28979 as separate changes so each could have focused implementation, measurable evidence, and regression coverage. I'm unsure how overlapping PRs are handled here, so I'll leave that decision to @ritchie46 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #28993 +/- ##
==========================================
+ Coverage 81.32% 81.35% +0.03%
==========================================
Files 1886 1886
Lines 267442 267458 +16
Branches 3061 3061
==========================================
+ Hits 217497 217593 +96
+ Misses 49167 49087 -80
Partials 778 778 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I have implemented the changes for this issue in #28993. Would appreciate a review when you get a chance. Thanks! |
|
@Angshuman09, I reviewed current diff against # 28979. Few things need to be addressed:
PR desc mostly repeats issue context and describes reservation fix already merged in # 28980, which obscures current scope and delay maintainer review. Cut that from desc, make scope clearer to speed up review. |
Closes #28979
Resolves #28872
Core Problem & Background
In Polars and Arrow,
FixedSizeListoperates simultaneously across two distinct coordinate spaces:inner_builderorvalues), where each outer list item containsBecause both outer and child counts are represented as primitive
usizeintegers, unit mismatch is invisible to the Rust compiler. Passing an outer unit where a child unit is expected compiles without warning or type error.The Failure Mode (#28872)
In
FixedSizeListArrayBuilder::reserveandMutableFixedSizeListArray::reserve:What went wrong:
additionalis in outer list units.self.validity.reserve(additional)is correct because the validity bitmap is indexed per outer list slot.self.inner_builder.reserve(additional)is wrong becauseinner_builderstores child elements. Reserving onlyadditionalchild slots instead ofadditional * self.sizeallocates onlyConcrete Example:
For a$K = 16$ :
FixedSizeListwith widthDesign & Solution
Instead of scattering raw
outer * self.sizemultiplications across operations (where omissions can easily happen again), this PR centralizes all coordinate arithmetic into explicit conversion primitives.1. Centralized Primitives
Introduced
child_offsetandchild_lengthincrates/polars-arrow/src/array/fixed_size_list/mod.rs:2. Fixed Builders Pre-Allocation
Updated
reservein both builders to convert outer units before delegating to the child buffer:FixedSizeListArrayBuilder:MutableFixedSizeListArray:Verification
Added Regression Tests
crates/polars/tests/it/arrow/array/fixed_size_list/mutable.rs:test_reserve: Verifies thatMutableFixedSizeListArray::reserve(10)with width 3 allocates at least 30 child units in the underlying buffer.crates/polars/tests/it/arrow/array/fixed_size_list/mod.rs:test_builder: TestsFixedSizeListArrayBuilderend-to-end (pre-allocation withreserve,extend_nulls,subslice_extend, andfreeze_reset).Test Results
cargo test -p polars --test it fixed_size_list
